Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Extract dylib for Mac, Remove Zip Requirement #65

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

Extract dylib for Mac, Remove Zip Requirement #65

wants to merge 1 commit into from

Conversation

muandrew
Copy link
Contributor

  • remove some unused imports.
  • replace computed path delimiters with Java provided constants.

@muandrew
Copy link
Contributor Author

tested on mac and ubuntu, please test on windows.

adakitesystems
adakitesystems reviewed Aug 20, 2018
View reviewed changes
Copy link
Collaborator

@adakitesystems adakitesystems left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR, @muandrew!

Overall, I like the ideas in the PR such as:

  • The removal of the third-party zip lib dependency
  • Support for Mac
  • Reducing the amount of code

I dislike some of the coding decisions on which I have commented in the review.

Lastly, these changes do not work on Windows + Vanilla bridge. I haven't tested it on Linux but I suspect it would fail on Linux too because the changes in this PR do not extract from an external JAR library during bot development since they assume a bundled BWAPI4J library jar + bot combination.

[DEBUG] [main] openbw.bwapi4j.BW:extractBridgeDependencies:224 - Extracting dependencies from: C:\projects\Raphael\raphael\libs\BWAPI4J-0.8.2b.jar
[FATAL] [main] openbw.bwapi4j.BW:extractBridgeDependencies:234 - Failed to extract dependencies from JAR.
java.lang.NullPointerException
	at java.util.Objects.requireNonNull(Objects.java:203)
	at java.nio.file.Files.copy(Files.java:2984)
	at org.openbw.bwapi4j.BW.extractFileIfNotExists(BW.java:245)
	at org.openbw.bwapi4j.BW.extractBridgeDependencies(BW.java:226)
	at org.openbw.bwapi4j.BW.loadSharedLibraries(BW.java:291)
	at org.openbw.bwapi4j.BW.<init>(BW.java:152)
	at org.openbw.bwapi4j.BW.<init>(BW.java:128)
	at com.github.adakitesystems.raphael.Raphael.main(Raphael.java:271)

BWAPI4J/src/main/java/org/openbw/bwapi4j/BW.java
&& !Files.exists(targetFile)) {
zipFile.extractFile(sourceFilename, targetDirectory);
private static void extractFileIfNotExists(final String sourceFilename, final String targetDirectory) {
final File library = new File(targetDirectory, sourceFilename);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Path is more modern and robust than File. https://docs.oracle.com/javase/tutorial/essential/io/legacy.html . Please provide justification for using File over Path in this case.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't aware of the changes but will migrate to using Path if I am able to convert to using getResourceAsStream.

BWAPI4J/src/main/java/org/openbw/bwapi4j/BW.java
InputStream is = BW.class.getResourceAsStream(File.separator + sourceFilename);
try {
Files.copy(is, library.toPath(), StandardCopyOption.REPLACE_EXISTING);
} catch (IOException e) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the justification for catching an IOException and not rethrowing it? This makes debugging more difficult as the library will fail to load and we won't know exactly why. Here, we would be able to see it's not extracting properly. It should be thrown in some way. It could be wrapped in an runtime exception. E.g. throw new RuntimeException(e);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error can be re-thrown on cleanup, but I think I'll remove this since I'd have to test external jar first.

BWAPI4J/src/main/java/org/openbw/bwapi4j/BW.java
@@ -331,11 +330,6 @@ private void loadLibraries(final List<String> libraries) {

return libNames;
}

private static String getLibraryPathDelimiter() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are now at the mercy of the JDK/JRE deciding what the proper delimiter is. But, I am willing to try File.pathSeparator out.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have there been instances where the JVM is detecting the wrong system? If so I would just consolidate the methods since the logic to determine the delimiter is duplicated.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No instances that I know of nor for which I have any evidence. As I said, we can try it out no big deal and it's probably fine. I just like having more control over simple things rather than relying on a library (even the JDK) to probe the system for something simple.

@muandrew
Copy link
Contributor Author

Hey! Can you refer me to a setup where an external jar is used? I'll test it out before including the Zip file removal changes.

@adakitesystems
Copy link
Collaborator

Can you refer me to a setup where an external jar is used? I'll test it out before including the Zip file removal changes.

The stacktrace was generated from the following setup:

  • Create a new BWAPI4J Java bot project using the TestListener.java bot setup as a reference guide (might be able to just use the entire single file).
  • Compile or obtain the BWAPI4J library (BWAPI4J-v0.8.2b.jar) and import it into the project as a JAR dependency.
  • Run the bot.

@muandrew
Copy link
Contributor Author

So when I'm testing I'm using a gradle setup. In the setup you described, is the project packaged somehow or does it use a different build system?

apply plugin: 'application'
apply plugin: 'kotlin'

mainClassName = 'com.muandrew.yari.KRunnerKt'

dependencies {
    implementation fileTree(dir: 'libs', include: ['*.jar'])
    implementation 'net.lingala.zip4j:zip4j:1.2.5'
    implementation 'org.apache.logging.log4j:log4j-core:2.11.0'

    implementation project(':lib')

    implementation "org.jetbrains.kotlin:kotlin-stdlib-jdk8:$kotlin_version"

    testImplementation 'junit:junit:4.12'
}

@adakitesystems
Copy link
Collaborator

Yes, the project is packaged as a JAR with the bridge inside. For example, the following Gradle tasks will build and package BWAPI4J as a distributed library JAR to be able to be imported into a project as described above in my post or with Gradle as described in your post if the C++ prereqs are installed on the system. From the root project directory:

git submodule update --init --recursive
cd BWAPI4J/
./gradlew clean build sourcesJar javadocJar buildOpenBWBridgeForLinux
./gradlew shadowJar

The resulting JARs will be found in build/libs/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adakitesystems adakitesystems adakitesystems left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@muandrew @adakitesystems